Skip to content

Reuse a per-connection encode buffer for daemon replies - #3157

Closed
phil-opp wants to merge 1 commit into
followup/datamessage-serialize-bytesfrom
followup/per-connection-encode-buffer
Closed

Reuse a per-connection encode buffer for daemon replies#3157
phil-opp wants to merge 1 commit into
followup/datamessage-serialize-bytesfrom
followup/per-connection-encode-buffer

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Stacked on #<FOLLOWUP1_PR> (followup/datamessage-serialize-bytes), which is itself stacked on the postcard migration. Review those first.

Read the measurement before the diff — you may want to close this instead of merging it.

TcpConnection::send_reply allocated a fresh Vec per reply. Since it already takes &mut self, the buffer can live on the connection: dora_message::encode_into takes a buffer and returns it, so the connection hands the same one back each time.

What it's actually worth

I measured this rather than trusting the estimate in the issue, which guessed 30–100 ns. It's less than half the bottom of that range:

payload fresh alloc reused buffer saved share of encode
0 B 59.5 ns 51.4 ns 8.0 ns 14%
64 B 67.7 ns 56.2 ns 11.5 ns 17%
4 KB 100.8 ns 86.1 ns 14.7 ns 15%

So: ~15% of encode cost, but encode is a small part of send_reply, which then does an async socket write measured in microseconds. In context this is well under 1% of the operation.

My read: this is below the bar for the state it adds, and I'd understand closing it. It's in your hands rather than mine because you asked for the PR — I'm giving you the numbers to decide on, not a recommendation to merge. If you do close it, dora_message::encode_into and its test are the only pieces I'd suggest keeping, and only if something else wants them.

If it does land

The retained buffer is capped at MAX_RETAINED_SEND_BUF (256 KiB). Without that, a single outsized reply would pin its full size — up to MAX_MESSAGE_BYTES, 64 MiB — for the life of the connection, and the daemon holds one connection per node. That's the one way this change could do real harm, so it's bounded explicitly rather than left to Vec's growth policy.

The buffer is also returned to the connection on the send-failure path, so a transient socket error doesn't silently drop the reuse for the rest of the connection's life.

encode_into_ignores_the_buffers_previous_contents covers the failure mode that would actually hurt: a missing clear() leaving stale bytes in a frame. It feeds in buffers that are empty, shorter, exactly equal, 4x longer, and large-but-empty-with-capacity, and asserts the output is byte-identical to a fresh encode and still decodes.

Scope

Only the daemon reply path, where &mut self already exists. The node side (apis/rust/node/src/daemon_connection/tcp.rs::send_message) is a free function over &mut TcpStream, so the buffer would have to move onto the caller's connection struct — more churn than the measured win justifies, so I left it.

Verification

  • cargo fmt --all -- --check, cargo clippy --all -- -D warnings — clean
  • cargo test --all — 125 test binaries, zero failures
  • Timings above from a release-mode harness encoding a real DaemonRequest::SendMessage, fresh-allocation vs held-buffer, same machine

`TcpConnection::send_reply` allocated a fresh `Vec` per reply. It
already takes `&mut self`, so the buffer can live on the connection:
`dora_message::encode_into` takes a buffer and hands it back.

Measured, rather than estimated -- and it is smaller than the estimate
that motivated the idea (30-100 ns):

    payload    fresh    reused    saved   share of encode
       0 B    59.5ns    51.4ns    8.0ns       14%
      64 B    67.7ns    56.2ns   11.5ns       17%
      4 KB   100.8ns    86.1ns   14.7ns       15%

~15% of encode cost, but encode is a small part of `send_reply`, which
then awaits a socket write measured in microseconds -- so under 1% of
the operation. This is arguably below the bar for the state it adds;
the PR body says so explicitly so the call can be made on the numbers.

The retained buffer is capped at MAX_RETAINED_SEND_BUF (256 KiB).
Without that a single outsized reply would pin its full size -- up to
MAX_MESSAGE_BYTES, 64 MiB -- for the life of the connection, and the
daemon holds one connection per node. The buffer is also returned on
the send-failure path, so a transient socket error does not silently
drop the reuse for the rest of the connection.

`encode_into_ignores_the_buffers_previous_contents` covers the failure
mode that would actually hurt: a missing `clear()` leaving stale bytes
in a frame. It feeds in buffers that are empty, shorter, exactly equal,
4x longer, and large-but-empty-with-capacity, and asserts the output is
byte-identical to a fresh `encode` and still decodes.

Scoped to the daemon reply path, where `&mut self` already exists. The
node side is a free function over `&mut TcpStream`, so its buffer would
have to move onto the caller's connection struct -- more churn than the
measured win justifies.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant